-
Notifications
You must be signed in to change notification settings - Fork 335
[vcpkg-tool] Replace curl command calls with libcurl #1660
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
dedafaf to
8a25177
Compare
64cfadf to
cddb978
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR replaces the existing curl command line tool usage with direct libcurl API calls throughout the vcpkg codebase. The goal is to improve reliability, error handling, and reduce external dependencies by using libcurl directly instead of spawning curl subprocesses.
Key changes include:
- Adding new
libcurlinfrastructure with proper initialization and error handling - Replacing all
curlcommand-line invocations withlibcurlAPI calls in download, upload, and metrics functionality - Making the
z-upload-metricscommand work on non-Windows platforms by removing the Windows-only restriction
Reviewed Changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/vcpkg/metrics.cpp | Replaces WinHTTP metrics upload with libcurl implementation and updates background process execution |
| src/vcpkg/commands.z-upload-metrics.cpp | Removes Windows-only restriction and adds file deletion after upload |
| src/vcpkg/base/downloads.cpp | Replaces all curl command-line calls with libcurl API calls for downloads, uploads, and HTTP requests |
| src/vcpkg/base/curl.cpp | Adds new libcurl global initialization infrastructure |
| include/vcpkg/base/curl.h | Provides libcurl header includes and initialization function |
| src/vcpkg/base/files.cpp | Adds file size methods to ReadFilePointer for libcurl upload functionality |
| CMakeLists.txt | Adds libcurl dependency and linking configuration |
| locales/messages.json | Updates error message formats to match libcurl error reporting |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| maybe_accepted->is_integer() && maybe_errors->is_array()) | ||
| { | ||
| auto item_received = maybe_received->integer(VCPKG_LINE_INFO); | ||
| auto item_accepted = maybe_accepted->integer(VCPKG_LINE_INFO); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a fan of this check followed by assert pattern but there's currently no maybe_integer so no change requested for now.
| set(CMAKE_DISABLE_FIND_PACKAGE_ZLIB ON) | ||
| set(CMAKE_DISABLE_FIND_PACKAGE_LibPSL ON) | ||
| set(CMAKE_DISABLE_FIND_PACKAGE_LibSSH2 ON) | ||
| set(CMAKE_DISABLE_FIND_PACKAGE_Brotli ON) | ||
| set(CMAKE_DISABLE_FIND_PACKAGE_Zstd ON) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another reason to consider hosting ourselves rather than FetchContent would be to more easily support ZLIB for Content-encoding: deflate or gzip.
Co-authored-by: Billy O'Neal <[email protected]> Co-authored-by: Thomas1664 <[email protected]>
Summary by Copilot:
This PR replaces the existing
curlcommand line tool usage with directlibcurlAPI calls throughout the vcpkg codebase. The main goal is to improve reliability, error handling, and reduce external dependencies by usinglibcurldirectly instead of spawningcurlsubprocesses.libcurlheaders on Linux CI runners.curlcommand-line invocations withlibcurlAPI callslibcurlinfrastructure with proper initialization and error handlinglibcurlerror message formatz-upload-metricswork on non-Windows platformsBefore merging these changes, we require confirmation from telemetry that the large majority of non-Windows users have a system-installed
libcurl.TODO: